Skip to content

Physical scale: detect the audited shape (no unit / [1,1,1]) and use the manifest pitch - #2220

Closed
gaogao94 wants to merge 1 commit into
ScrollPrize:mainfrom
gaogao94:corrected-scale-fix
Closed

gaogao94 wants to merge 1 commit into
ScrollPrize:mainfrom
gaogao94:corrected-scale-fix

Conversation

@gaogao94

Copy link
Copy Markdown

Could a maintainer add the keep-open label? The last attempt (#2137) was closed by the PR time-limit
rule before anyone could act on the review; this one is the corrected version, and the label is what keeps it
alive long enough to be reviewed.

What was wrong with #2137, and what changed

@pmh47 was right on all three points: the audited stores publish an explicit [1,1,1] scale with no unit,
so a check that only fires when the scale transform is missing never warns on them; and the
metadata.min.json fallback the description promised was not in the diff at all. Both are fixed here.

  1. Check the real condition. _axes_have_units() reads multiscales[0].axes[].unit and is independent of
    whether a transform is present; a level-0 scale of exactly [1, 1, 1] is also reported separately, since
    on a store whose name states a pitch that value is a placeholder rather than a measurement.
  2. Implement the fallback. _pitch_from_manifest() resolves the store against metadata.min.json and
    reads properties.pixel_size_um. When the catalog records a pitch the warning names it as the value to
    use; when it does not (the six legacy fragments) the warning says so instead.
  3. Tests pin the audited shape, not the missing-transform shape: units absent + explicit [1,1,1] must
    warn, units present must stay quiet, a channel axis without a unit must not count. A test that only covers
    the missing-transform case is exactly how the first version passed while doing nothing.

Calibration against the live catalog

PHerc0009B/volumes/20250521125136-8.640um-1.2m-116keV-masked.zarr  -> 8.64   (manifest pixel_size_um)
PHerc0009B/volumes/20250820154339-2.401um-0.3m-77keV-masked.zarr   -> 2.401
a store the manifest records no pitch for                         -> None

and the published metadata of that first store is the audited shape exactly:

axes:    [('z', None), ('y', None), ('x', None)]
level 0: [{'scale': [1.0, 1.0, 1.0], 'type': 'scale'}]

State of the diff: rebased onto main at 13a88a6, 10 files, +194/-2 in the first version and the
corrections above on top; every touched file compiles and the new tests are in lasagna/tests/. What I could
not do is run the repository's test suite here - the dependency chain is deep enough that I could not
reproduce it - so the CI run on this PR is the check I most want, and if anything fails I will fix it.

If the reviewer's preference is a different shape - for example failing hard rather than warning when the
catalog records a pitch and the store does not - say which and I will change it; the point of the patch is to
make the audited stores impossible to miss, not to prescribe a policy.

The first version of this patch (closed as ScrollPrize#2137) was wrong in the way the review said: it only
warned when the scale transform was missing entirely, while the stores the audit reports publish an
explicit [1,1,1] scale and no unit on any axis - so they passed silently. The metadata.min.json
fallback the description promised was not implemented either.

This replaces both: _axes_have_units() checks axes[].unit independently of the transform,
_pitch_from_manifest() reads properties.pixel_size_um from the catalog (verified against the live
catalog: 8.64, 2.401, and None for a store with no recorded pitch), and the tests pin the audited
shape instead of the missing-transform one.
@vercel

vercel Bot commented Oct 22, 2026

Copy link
Copy Markdown

@gaogao94 is attempting to deploy a commit to the scroll Team on Vercel.

A member of the Team first needs to authorize it.

@bruniss

bruniss commented Nov 1, 2026

Copy link
Copy Markdown
Member

tifxyz reading will be moved into vc_reader in a few days in a separate pr, which also handles this

@bruniss bruniss closed this Nov 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants